Skip to content

Fix overriding common_kwargs defaults in processor calls#41381

Merged
yonigozlan merged 3 commits into
huggingface:mainfrom
yonigozlan:fix-common-kwargs
Oct 8, 2025
Merged

Fix overriding common_kwargs defaults in processor calls#41381
yonigozlan merged 3 commits into
huggingface:mainfrom
yonigozlan:fix-common-kwargs

Conversation

@yonigozlan

Copy link
Copy Markdown
Contributor

What does this PR do?

After #40931, the common_kwargs (return_tensors really) defaults were set after updating them with call kwargs, which mean it's impossible to override the defaults.
This fixes it by setting the common kwargs defaults before updating them with call kwargs, and fixes a test in test_image_processing_owlv2.py that was failing because of it (as Owlv2 sets return_tensors to np by default for some reasons)

Cc @zucchini-nlp to make sure I'm not missing anything!

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Comment thread src/transformers/processing_utils.py Outdated
Comment on lines +1309 to +1310
common_kwargs = kwargs.get("common_kwargs", {})
common_kwargs.update(ModelProcessorKwargs._defaults.get("common_kwargs", {}))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case i think we want to change the order of these lines as well, instead of overriding user values by defaults

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes true, but should we even support that? It would be kind of weird for users to use a "common_kwargs" kwarg instead of just using return_tensors or other common_kwargs directly no?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i don't think anyone uses nested kwargs anyway. Just for correctness, if we support common kwargs, we should use whatever users can pass with priority imo

@zucchini-nlp zucchini-nlp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Approving in advance so it can be merged right after fixing the order once more

@yonigozlan yonigozlan enabled auto-merge (squash) October 8, 2025 03:02
@yonigozlan yonigozlan merged commit 3553f0b into huggingface:main Oct 8, 2025
25 checks passed
AhnJoonSung pushed a commit to AhnJoonSung/transformers that referenced this pull request Oct 12, 2025
…#41381)

* set common_kwargs defaults before updating with kwargs

* change order to override defaults common_kwargs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants